Open
Conversation
📝 WalkthroughWalkthroughAdds tracking and validation for type-hinted Method objects in BaseController, centralizes name-clash detection across attributes, methods, and sub-controllers, updates add/validation flows to enforce hinted constraints, and re-exports Method from the methods package; tests updated to cover new validations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
+ Coverage 90.84% 90.89% +0.05%
==========================================
Files 70 70
Lines 2555 2592 +37
==========================================
+ Hits 2321 2356 +35
- Misses 234 236 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shihab-dls
force-pushed
the
validate_methods
branch
from
747f2ad to
ab37703
Compare
GDYendell
requested changes
Feb 18, 2026
Contributor
GDYendell
left a comment
GDYendell
left a comment
There was a problem hiding this comment.
Minor comments and a question
Contributor
There was a problem hiding this comment.
Do we want the from when the base exception has no message? What does this look like? Maybe there should be a base message.
Contributor
Author
There was a problem hiding this comment.
Good point; the exceptions without from exc just don't have:
Traceback (most recent call last):
File "/workspaces/FastCS/src/fastcs/controllers/base_controller.py", line 212, in _validate_hinted_attribute
attr = self._validate_hinted_member(name, Attribute)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/workspaces/FastCS/src/fastcs/controllers/base_controller.py", line 192, in _validate_hinted_member
raise RuntimeError()
RuntimeError
The above exception was the direct cause of the following exception:
which is not must more informative than the rest of the exception:
Traceback (most recent call last):
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/_pytest/runner.py", line 353, in from_call
result: TResult | None = func()
^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/_pytest/runner.py", line 245, in <lambda>
lambda: runtest_hook(item=item, **kwds),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_hooks.py", line 512, in __call__
return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_manager.py", line 120, in _hookexec
return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 167, in _multicall
raise exception
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 139, in _multicall
teardown.throw(exception)
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/_pytest/logging.py", line 850, in pytest_runtest_call
yield
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 139, in _multicall
teardown.throw(exception)
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/_pytest/capture.py", line 900, in pytest_runtest_call
return (yield)
^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 139, in _multicall
teardown.throw(exception)
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 53, in run_old_style_hookwrapper
return result.get_result()
^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_result.py", line 103, in get_result
raise exc.with_traceback(tb)
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 38, in run_old_style_hookwrapper
res = yield
^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 139, in _multicall
teardown.throw(exception)
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/_pytest/skipping.py", line 268, in pytest_runtest_call
return (yield)
^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 121, in _multicall
res = hook_impl.function(*args)
^^^^^^^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/_pytest/runner.py", line 179, in pytest_runtest_call
item.runtest()
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/_pytest/python.py", line 1720, in runtest
self.ihook.pytest_pyfunc_call(pyfuncitem=self)
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_hooks.py", line 512, in __call__
return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_manager.py", line 120, in _hookexec
return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 167, in _multicall
raise exception
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 139, in _multicall
teardown.throw(exception)
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 53, in run_old_style_hookwrapper
return result.get_result()
^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_result.py", line 103, in get_result
raise exc.with_traceback(tb)
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 38, in run_old_style_hookwrapper
res = yield
^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/pluggy/_callers.py", line 121, in _multicall
res = hook_impl.function(*args)
^^^^^^^^^^^^^^^^^^^^^^^^^
File "/cache/venv-for/scratch/rye74444/FastCS/lib/python3.11/site-packages/_pytest/python.py", line 166, in pytest_pyfunc_call
result = testfunction(**testargs)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/workspaces/FastCS/tests/test_controllers.py", line 172, in test_attribute_hint_validation
controller._validate_type_hints()
File "/workspaces/FastCS/src/fastcs/controllers/base_controller.py", line 177, in _validate_type_hints
self._validate_hinted_attribute(name)
File "/workspaces/FastCS/src/fastcs/controllers/base_controller.py", line 214, in _validate_hinted_attribute
raise RuntimeError(
RuntimeError: Controller `HintedController` failed to introspect hinted attribute `read_write_int` during initialisation
so I think we just:
raise RuntimeError(
f"Controller `{self.__class__.__name__}` failed to introspect "
f"hinted attribute `{name}` during initialisation"
) from Nonesince there isn't more context the base exception needs to provide
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/fastcs/controllers/base_controller.py (3)
188-193:member is Nonecheck is redundant
isinstance(None, expected_type)already evaluates toFalsefor all relevant types (Attribute,Method,BaseController), making the explicitNoneguard superfluous.♻️ Proposed simplification
def _validate_hinted_member(self, name: str, expected_type: type): """Validate that a hinted member exists on the controller""" member = getattr(self, name, None) - if member is None or not isinstance(member, expected_type): + if not isinstance(member, expected_type): raise RuntimeError() return member🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/controllers/base_controller.py` around lines 188 - 193, The check "member is None" in _validate_hinted_member is redundant because isinstance(None, expected_type) is always False; update the function (named _validate_hinted_member) to remove the explicit None guard and simply use "if not isinstance(member, expected_type): raise RuntimeError(...)" so it still raises for missing/wrong types (e.g., Attribute, Method, BaseController) and optionally include a helpful message in the RuntimeError for debugging.
347-380: Type-mismatch detail from_validated_added_methodis swallowed by the error wrapperBoth
_check_for_name_clashand_validated_added_methodsit inside the sametryblock, so the rich "ExpectedCommandgotScan" message from_validated_added_methodis replaced by the generic "Cannot add command/scan method X." — it's only recoverable via__cause__.
add_attributeavoids this by placing the hint-type check outside the try block, preserving its message as the primary exception. Aligningadd_command/add_scanwith that pattern would make type mismatch errors immediately actionable:♻️ Proposed fix for `add_command` and `add_scan`
def add_command(self, name: str, command: Command): try: self._check_for_name_clash(name) except ValueError as exc: raise ValueError(f"Cannot add command method {command}.") from exc + self._validated_added_method(name, command) - try: - self._check_for_name_clash(name) - self._validated_added_method(name, command) - except (ValueError, RuntimeError) as exc: - raise exc.__class__(f"Cannot add command method {command}.") from exc self.__command_methods[name] = command super().__setattr__(name, command) def add_scan(self, name: str, scan: Scan): try: self._check_for_name_clash(name) except ValueError as exc: raise ValueError(f"Cannot add scan method {scan}.") from exc + self._validated_added_method(name, scan) - try: - self._check_for_name_clash(name) - self._validated_added_method(name, scan) - except (ValueError, RuntimeError) as exc: - raise exc.__class__(f"Cannot add scan method {scan}.") from exc self.__scan_methods[name] = scan super().__setattr__(name, scan)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/controllers/base_controller.py` around lines 347 - 380, The try block in add_command and add_scan currently wraps both _check_for_name_clash and _validated_added_method so a RuntimeError from _validated_added_method is masked; change the flow to call _check_for_name_clash inside a try/except that only catches and re-raises the name-clash ValueError, then call _validated_added_method(name, ...) outside that try so its RuntimeError (the detailed "Expected X got Y" message) bubbles up as the primary exception; apply the same pattern to both add_command and add_scan and keep using __command_methods/__scan_methods and super().__setattr__ as before.
200-203: Ruff TRY003: Long exception messages defined outside exception classesAll the new/changed
raise RuntimeError(…)andraise ValueError(…)sites across these lines are flagged by Ruff TRY003. The conventional fix is to define dedicated exception subclasses (e.g.,ControllerInitialisationError) and put the message there, though given the scope this is a non-blocking style preference.Also applies to: 214-217, 228-231, 285-287, 293-293, 324-324, 351-356, 363-363, 377-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/controllers/base_controller.py` around lines 200 - 203, Create dedicated exception subclasses (e.g., ControllerInitialisationError and ControllerIntrospectionError) in this module and move the long messages into those classes (as default message in class body or __init__) instead of constructing long strings at each raise site; then replace occurrences of raise RuntimeError(...) and raise ValueError(...) (including the one using self.__class__.__name__ and the hinted method `name`, and the other flagged sites) with raising the appropriate new exception class (preserve exception chaining like `from None` where used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fastcs/controllers/base_controller.py`:
- Line 186: The suppression comment `# noqa: SLF001` on the call to
subcontroller._validate_type_hints() is unused; remove the `# noqa: SLF001`
suffix from that line so the call is simply subcontroller._validate_type_hints()
and run linting to confirm no further suppressions for SLF001 are present.
---
Nitpick comments:
In `@src/fastcs/controllers/base_controller.py`:
- Around line 188-193: The check "member is None" in _validate_hinted_member is
redundant because isinstance(None, expected_type) is always False; update the
function (named _validate_hinted_member) to remove the explicit None guard and
simply use "if not isinstance(member, expected_type): raise RuntimeError(...)"
so it still raises for missing/wrong types (e.g., Attribute, Method,
BaseController) and optionally include a helpful message in the RuntimeError for
debugging.
- Around line 347-380: The try block in add_command and add_scan currently wraps
both _check_for_name_clash and _validated_added_method so a RuntimeError from
_validated_added_method is masked; change the flow to call _check_for_name_clash
inside a try/except that only catches and re-raises the name-clash ValueError,
then call _validated_added_method(name, ...) outside that try so its
RuntimeError (the detailed "Expected X got Y" message) bubbles up as the primary
exception; apply the same pattern to both add_command and add_scan and keep
using __command_methods/__scan_methods and super().__setattr__ as before.
- Around line 200-203: Create dedicated exception subclasses (e.g.,
ControllerInitialisationError and ControllerIntrospectionError) in this module
and move the long messages into those classes (as default message in class body
or __init__) instead of constructing long strings at each raise site; then
replace occurrences of raise RuntimeError(...) and raise ValueError(...)
(including the one using self.__class__.__name__ and the hinted method `name`,
and the other flagged sites) with raising the appropriate new exception class
(preserve exception chaining like `from None` where used).
GDYendell
reviewed
Feb 18, 2026
| for subcontroller in self.sub_controllers.values(): | ||
| subcontroller._validate_type_hints() # noqa: SLF001 | ||
|
|
||
| def _validate_hinted_member(self, name: str, expected_type: type): |
Contributor
There was a problem hiding this comment.
This is missing a return type. I think it should be
Suggested change
| def _validate_hinted_member(self, name: str, expected_type: type): | |
| def _validate_hinted_member(self, name: str, expected_type: T) -> T: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #300
This PR validated hinted scan and command methods, and splits validation logic into helper functions.
Summary by CodeRabbit
New Features
Improvements
Tests